-
Notifications
You must be signed in to change notification settings - Fork 14.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
style: DOCTYPE tag, and related CSS cleanup/refactoring #10302
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10302 +/- ##
==========================================
+ Coverage 65.46% 69.65% +4.18%
==========================================
Files 603 196 -407
Lines 32436 19060 -13376
Branches 3297 0 -3297
==========================================
- Hits 21234 13276 -7958
+ Misses 11018 5784 -5234
+ Partials 184 0 -184
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This is a much needed change!
align-content: stretch; | ||
div:last-of-type { | ||
flex-basis: 100%; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is breaking FilterBox in Explore page. @rusackas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang! Thanks for the quick catch. I'll take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get to fix it entirely last night... but it's not just that style... there are some dimensions that need to be added to a div... they're being added to other viz components, but not this one for some reason. Hope to have a PR soon.
<Styles className="chart-container"> | ||
<div>{header}</div> | ||
<div>{this.renderChart()}</div> | ||
</Styles> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rusackas by not using the Panel
component, you also missed some CSS styles for .panel-heading
and .panel-body
.
After
Before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed a fix. I'm using the raw boostrap classes for now. Let's refactor it into Antd components or a more self-contained styled component in the future.
FYI this PR broke thumbnails for alerts #10488 |
Impacts #8976 |
SUMMARY
Superset has been running in quirks mode due to a lack of DOCTYPE tag. This adds the DOCTYPE, which causes various CSS issues. This code also fixes that CSS. CSS fixes led to some Emotion styles, removing some JS-calculated dimensions, leaning on Flexbox more, and removing at least one Bootstrap Panel component. Hopefully we can continue to simplify and speed up rendering using these techniques. UI should remain effectively unchanged.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Made a change along the way, to have control panels scroll within the control tabs, so you don't have to scroll all the way up to switch tabs. Been meaning to do this forever anyway. Here's the AFTER gif:
TEST PLAN
ADDITIONAL INFORMATION